feat: add typings for <Button> #3080
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3080 +/- ##
==========================================
+ Coverage 93.19% 93.20% +0.01%
==========================================
Files 249 249
Lines 4348 4359 +11
Branches 1000 1036 +36
==========================================
+ Hits 4052 4063 +11
+ Misses 293 290 -3
- Partials 3 6 +3 ☔ View full report in Codecov by Sentry. |
| render(<Button as={Hyperlink} destination="https://www.poop.com/💩">Button</Button>); | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const ref = (_current: HTMLAnchorElement) => {}; // Check typing of a ref - should not show type errors. | ||
| render(<Button as={Hyperlink} ref={ref} destination="https://www.poop.com/💩">Button</Button>); |
There was a problem hiding this comment.
This shows an example of how passing as={Hyperlink} changes the types of various other props like ref to match.
| @@ -30,7 +30,7 @@ describe('<Button />', () => { | |||
|
|
|||
| it('renders with props iconAfter and size', () => { | |||
| const tree = renderer.create(( | |||
| <Button iconAfter={Close} size="md">Button</Button> | |||
| <Button iconAfter={Close} size="sm">Button</Button> | |||
There was a problem hiding this comment.
I just changed this to sm for some variation - too many of the tests were just using the same md size.
src/Button/index.tsx
Outdated
| import BaseButton, { type ButtonProps as BaseButtonProps } from 'react-bootstrap/Button'; | ||
| import BaseButtonGroup, { type ButtonGroupProps as BaseButtonGroupProps } from 'react-bootstrap/ButtonGroup'; | ||
| import BaseButtonToolbar, { type ButtonToolbarProps } from 'react-bootstrap/ButtonToolbar'; | ||
| import { type BsPrefixRefForwardingComponent as ComponentWithAsProp } from 'react-bootstrap/esm/helpers'; |
There was a problem hiding this comment.
[question] Given this pattern of importing BsPrefixRefForwardingComponent (and BsPrefixProps, see #3082), I wonder if there's a more common/global location to define these types when renaming them so each individual component doesn't need to do the same re-naming repeatedly (e.g., risking different naming across components).
For example, Hyperlink (in #3082) will also rely on this pattern so it'd be nice to only rename it in one place.
There was a problem hiding this comment.
Yes, that's a good idea and I'm happy to move it somewhere shared. Do you have a place in mind or want me to make something up?
There was a problem hiding this comment.
First thing that came to my mind was to rename https://github.com/openedx/paragon/tree/master/src/utils/propTypes to src/utils/types and then put it in there, but I'm very open to any other ideas.
There was a problem hiding this comment.
OK, I didn't rename propTypes for now since I want to distinguish them from types (but feel free to do that later if you want). But otherwise I did this: 66f85cb
I took the opportunity to add lots of documentation to explain how these types work (since the Bootstrap types are pretty obscure), and added in some type-only tests to verify that my understanding of them is correct (and support any future refactors).
bradenmacdonald
force-pushed
the
type-button
branch
from
23a1891
to
66f85cb
Compare
|
I believe codecov is stuck, I'm going to merge this now instead of waiting for that check
|
|
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
🎉 This PR is included in version 22.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Thanks @brian-smith-tcril ! |
* feat: add typings for <Button> * chore: bump @types/react and @types/react-dom * feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
* feat: add typings for <Button> * chore: bump @types/react and @types/react-dom * feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
* feat: add typings for <Button> * chore: bump @types/react and @types/react-dom * feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
* feat: add typings for <Button> * chore: bump @types/react and @types/react-dom * feat: re-export bootstrap helpers as ComponentWithAsProp, BsPropsWithAs
Description
This adds typescript types for
<Button>as well as<ButtonGroup>and<ButtonToolbar>.I tested it against various usages of
<Button>in frontend-app-course authoring, as well as all the usages in this repo itself, and couldn't find any issues.Deploy Preview
https://deploy-preview-3080--paragon-openedx.netlify.app
Merge Checklist
exampleapp?Post-merge Checklist